gh-95385 Fastpath for encoding dict to JSON by aivarsk · Pull Request #95374 · python/cpython

aivarsk · 2022-07-28T10:51:06Z

When sorting of keys is not requested and we are encoding a dict
use PyDict_Next to iterate over keys and values.
When sorting is requested use PyList_GET_ITEM instead of PyIter_Next because we know we are working with a list.

Issue: Fastpath for encoding unsorted dict to JSON #95385

aivarsk · 2022-07-28T14:12:19Z

The code is based on another PR (gh-95382) not merged yet just to avoid passing extra parameters around.

corona10

Please separate the patch from removing indent_level params.

bedevere-bot · 2022-07-28T17:41:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

When sorting of keys is not requsted and we are encoding a dict use PyDict_Next to iterate over keys and values. Leave the old path with PyMapping_Items that creates a list of key-value tuples for cases when sorting is requested. Don't use mapping items and iterator: we check and know we are using a dict and list.

aivarsk · 2022-07-28T20:25:09Z

I have made the requested changes; please review again

bedevere-bot · 2022-07-28T20:25:12Z

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

corona10

Would you like to provide a benchmark script and result for the following conditions?

json.dumps when sorting of key is required
json.dumps when sorting of key is not required (your suggestion)

aivarsk · 2022-07-29T06:33:41Z

This is without sorting of keys, the default behavior of json.dumps
pyperformance on my machine before changes:

### json_dumps ###
Mean +- std dev: 10.6 ms +- 0.1 ms

pyperformance after changes:

### json_dumps ###
Mean +- std dev: 9.00 ms +- 0.08 ms

aivarsk · 2022-07-29T06:39:46Z

With pyperformance sort_keys=True, I edited benchmarks/bm_json_dumps/run_benchmark.py for that

before changes

### json_dumps ###
Mean +- std dev: 14.0 ms +- 0.2 ms

after changes (PyList_GET_ITEM is a bit faster than PyIter_Next?)

### json_dumps ###
Mean +- std dev: 13.6 ms +- 0.2 ms

corona10 · 2022-07-29T08:48:13Z

@aivarsk Thank you, I will take a look at this PR through this weekend :)

bedevere-bot · 2022-08-03T09:58:01Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 9137b1b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

corona10

Sorry for the delay, I should check several possibilities of regression including leaks but not found :)

Overall looks good LGTM, but please follow PEP 7 if possible.
I just left nit comments :)

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

bedevere-bot added the awaiting review label Jul 28, 2022

aivarsk force-pushed the dict_to_json_fastpath branch 2 times, most recently from 4d40abf to e2929e7 Compare July 28, 2022 13:11

aivarsk changed the title ~~gh-NNNNN Fastpath for encoding dict to JSON~~ gh-95385 Fastpath for encoding dict to JSON Jul 28, 2022

aivarsk marked this pull request as ready for review July 28, 2022 14:30

corona10 requested changes Jul 28, 2022

View reviewed changes

bedevere-bot removed the awaiting review label Jul 28, 2022

bedevere-bot added the awaiting changes label Jul 28, 2022

aivarsk added 2 commits July 28, 2022 23:22

NEWS

9137b1b

aivarsk force-pushed the dict_to_json_fastpath branch from 6e92af6 to 9137b1b Compare July 28, 2022 20:23

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 28, 2022

bedevere-bot requested a review from corona10 July 28, 2022 20:25

corona10 reviewed Jul 29, 2022

View reviewed changes

aivarsk commented Jul 29, 2022

View reviewed changes

Comment thread Modules/_json.c

corona10 self-assigned this Jul 29, 2022

mdboom added the performance Performance or resource usage label Aug 1, 2022

corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 3, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 3, 2022

corona10 reviewed Aug 3, 2022

View reviewed changes

Comment thread Modules/_json.c

corona10 reviewed Aug 3, 2022

View reviewed changes

Comment thread Modules/_json.c Outdated

Comment thread Modules/_json.c Outdated

aivarsk added 2 commits August 3, 2022 20:34

explicit checks

751e042

smaller diff

8ae1d21

aivarsk requested a review from corona10 August 4, 2022 09:43

corona10 reviewed Aug 5, 2022

View reviewed changes

Comment thread Modules/_json.c Outdated

corona10 reviewed Aug 5, 2022

View reviewed changes

Comment thread Modules/_json.c

Use PyDict_Items

7677295

corona10 approved these changes Aug 5, 2022

View reviewed changes

Comment thread Modules/_json.c Outdated

Comment thread Modules/_json.c Outdated

Comment thread Modules/_json.c Outdated

Comment thread Modules/_json.c Outdated

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 5, 2022

aivarsk and others added 4 commits August 5, 2022 23:03

Update Modules/_json.c

a21f77a

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

Update Modules/_json.c

4734d70

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

Update Modules/_json.c

79e6880

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

Update Modules/_json.c

fda2ee7

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

corona10 merged commit 15f4a35 into python:main Aug 6, 2022

bedevere-bot removed the awaiting merge label Aug 6, 2022

iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request Aug 11, 2022

pythongh-95385 Fastpath for encoding dict to JSON (pythongh-95374)

2d2e23a

Uh oh!

Conversation

aivarsk commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aivarsk commented Jul 28, 2022

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 28, 2022

Uh oh!

aivarsk commented Jul 28, 2022

Uh oh!

bedevere-bot commented Jul 28, 2022

Uh oh!

corona10 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivarsk commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aivarsk commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corona10 commented Jul 29, 2022

Uh oh!

bedevere-bot commented Aug 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aivarsk commented Jul 28, 2022 •

edited

Loading

corona10 left a comment •

edited

Loading

aivarsk commented Jul 29, 2022 •

edited

Loading

aivarsk commented Jul 29, 2022 •

edited

Loading